feat(console): add subcommand for gen completions #336

nrskt · 2022-04-26T18:06:28Z

This PR includes adding subcommand for shell completions. Generating a man page will be done as another PR.

❯ tokio-console
--ascii-only            --lang                  --no-duration-colors    --retain-for            -h                      gen-config
--colorterm             --log                   --no-terminated-colors  --version               <TARGET_ADDR>           help
--help                  --no-colors             --palette               -V                      gen-completion

❯ tokio-console gen-completion
--help      --version   -V          -h          bash        elvish      fish        powershell  zsh

Related to #325

hawkw · 2022-04-26T19:35:30Z

tokio-console/src/config.rs

+ /// It should be saved in expected directory, depending on the shell used
+ ///
+ /// $ tokio-console gen-completion zsh > $FPATH/_tokio_console


since we know the shell from the command-line argument, should we just make the command install the completions to the correct location, instead?

@hawkw Maybe it can install the completion but I think that It's better not to do it because the user wants to decide where is put.

hmm, isn't the completion location determined by the shell, rather than the user? i'm fine with the current approach, but think this is worth a potential follow-up.

hawkw · 2022-04-30T16:24:27Z

tokio-console/src/main.rs

+ if let Some(config::OptionalCmd::GenCompletion { shell }) = args.subcmd {
+ let mut app = config::Config::command();
+ generate(shell, &mut app, "tokio-console", &mut std::io::stdout());
+ return Ok(());
+ }


style, take it or leave it: now that we have multiple subcommands, we might want to consider collapsing this and the if block for the GenConfig subcommand into a match, like

match args.subcmd { Some(config::OptionalCmd::GenConfig) => // ... Some(config::OptionalCmd::GenCompletion { shell } => // ... None => {}, }

fixed: b51ce10

tokio-console/src/config.rs

hawkw · 2022-04-30T16:26:01Z

tokio-console/src/config.rs

+
+ /// Generate shell completions
+ ///
+ /// It should be saved in expected directory, depending on the shell used


maybe something like this:

Suggested change

/// It should be saved in expected directory, depending on the shell used

/// The completion script should be saved in the shell's completion directory.

/// This depends on which shell is in use.

///

/// For example, using zsh:

fixed: 60fbce5

hawkw · 2022-04-30T16:27:08Z

tokio-console/src/config.rs

+ /// It should be saved in expected directory, depending on the shell used
+ ///
+ /// $ tokio-console gen-completion zsh > $FPATH/_tokio_console


hmm, isn't the completion location determined by the shell, rather than the user? i'm fine with the current approach, but think this is worth a potential follow-up.

nrskt · 2022-05-02T06:50:23Z

@hawkw

hmm, isn't the completion location determined by the shell, rather than the user? i'm fine with the current approach, but think this is worth a potential follow-up.

Ok, I understand your opinion. I'll check how to completion directory the shell that I don't know (fish, elvish, powershell).
Should I fix it with another pull request? or continually this pull request?

nrskt · 2022-05-02T08:13:57Z

tokio-console/src/config.rs

+ ///
+ /// For example, using zsh:
+ ///
+ /// $ tokio-console gen-completion zsh > ~/.zsh_functions/_tokio_console


@hawkw
Sorry, I'm mistaken.
$FPATH has multiple path values so it's not work $FPATH/_tokio_console

I worried about this sample, because .zsh_functions directory name. This name is my config value on .zshrc. I don't know common name.

fa91ca9

hmm, if this isn't going to work for everyone, maybe we should remove the sample for now, and add it back later if we can come up with one that will work universally?

nrskt · 2022-05-02T08:33:32Z

tokio-console/src/main.rs

+fn gen_completion(install: bool, shell: Shell) -> color_eyre::Result<()> {
+ let mut app = config::Config::command();
+ let mut buf: Box<dyn std::io::Write> = if install {
+ let mut home_dir = dirs::home_dir()
+ .ok_or_else(|| color_eyre::eyre::eyre!("fail to find home directory"))?;
+ match shell {
+ Shell::Zsh => {
+ home_dir.push(".zsh_functions/_tokio_console");
+ let f = std::fs::File::create(&home_dir)
+ .wrap_err_with(|| format!("fail to open {}", home_dir.display()))?;
+ Box::new(std::io::BufWriter::new(f))
+ }
+ _ => color_eyre::eyre::bail!("Not support to install completion script on {}", shell),
+ }
+ } else {
+ Box::new(std::io::stdout())
+ };
+ generate(shell, &mut app, "tokio-console", &mut buf);
+ Ok(())
+}


@hawkw
How about this approach?
If the user runs as tokio-console gen-completion --install <SHELL> then put the script file.

But I must fix it, .zsh_functions directory is not correct. I have a problem.
I have tried to get a directory with $FPATH environment variables however I failed.

problem:
std::env::var("FPATH") return an error. I checked on my terminal like this:

❯ echo $FPATH /home/nrskt/.zsh_functions:....some paths ❯ env | rg FPATH (return nothing)

So FPATH not working in my environment. I can't determine if it's a problem with my environment or zsh features.

In my opinion, first I fix it to not support all shells like this:

match shell { _ => color_eyre::eyre::bail!("Not support to install completion script on {}", shell), }

And the rest should be treated for potential follow-up.

hmm...it seems like automatically installing completions may be a bit more complicated than i'd hoped. i think it's reasonable to just always fail if --install is passed, for now, so we can go ahead and merge this branch, and implement support for automatic installations later.

hawkw

Overall, this looks good! I had some small suggestions.

I think it's fine to save the automatic installation of completion scripts for later, since it looks like it's actually somewhat complex. For now, let's go with what you suggested and have that always fail, and the user can still install completions manually.

hawkw · 2022-05-05T18:27:35Z

tokio-console/src/main.rs

@@ -214,3 +223,24 @@ async fn watch_details_stream(
 }
 }
 }
+
+fn gen_completion(install: bool, shell: Shell) -> color_eyre::Result<()> {


nit, take it or leave it: I think it might make sense to put this function in the config module?

fixed: 99f659f

hawkw · 2022-05-05T18:30:29Z

tokio-console/src/main.rs

+fn gen_completion(install: bool, shell: Shell) -> color_eyre::Result<()> {
+ let mut app = config::Config::command();
+ let mut buf: Box<dyn std::io::Write> = if install {
+ let mut home_dir = dirs::home_dir()
+ .ok_or_else(|| color_eyre::eyre::eyre!("fail to find home directory"))?;
+ match shell {
+ Shell::Zsh => {
+ home_dir.push(".zsh_functions/_tokio_console");
+ let f = std::fs::File::create(&home_dir)
+ .wrap_err_with(|| format!("fail to open {}", home_dir.display()))?;
+ Box::new(std::io::BufWriter::new(f))
+ }
+ _ => color_eyre::eyre::bail!("Not support to install completion script on {}", shell),
+ }
+ } else {
+ Box::new(std::io::stdout())
+ };
+ generate(shell, &mut app, "tokio-console", &mut buf);
+ Ok(())
+}


hmm...it seems like automatically installing completions may be a bit more complicated than i'd hoped. i think it's reasonable to just always fail if --install is passed, for now, so we can go ahead and merge this branch, and implement support for automatic installations later.

hawkw · 2022-05-05T18:31:52Z

tokio-console/src/main.rs

+ } else {
+ Box::new(std::io::stdout())
+ };
+ generate(shell, &mut app, "tokio-console", &mut buf);


nit, take it or leave it: i would probably not import the generate function into this file globally, as it's not super clear what's being generated this way. i would probably have written

Suggested change

generate(shell, &mut app, "tokio-console", &mut buf);

clap_complete::generate(shell, &mut app, "tokio-console", &mut buf);

but, it's not a big deal either way.

fixed: 0c02ebf

tokio-console/src/main.rs

hawkw · 2022-05-05T18:33:44Z

tokio-console/src/main.rs

+ .wrap_err_with(|| format!("fail to open {}", home_dir.display()))?;
+ Box::new(std::io::BufWriter::new(f))
+ }
+ _ => color_eyre::eyre::bail!("Not support to install completion script on {}", shell),


maybe

Suggested change

_ => color_eyre::eyre::bail!("Not support to install completion script on {}", shell),

_ => color_eyre::eyre::bail!("Automatically installing completion scripts is not currently supported on {}", shell),

hawkw · 2022-05-05T18:34:44Z

tokio-console/src/config.rs

+ ///
+ /// For example, using zsh:
+ ///
+ /// $ tokio-console gen-completion zsh > ~/.zsh_functions/_tokio_console


hmm, if this isn't going to work for everyone, maybe we should remove the sample for now, and add it back later if we can come up with one that will work universally?

nrskt · 2022-05-07T06:32:03Z

@hawkw Fixed based on your suggestion. How about it?

hawkw

okay, this looks good to me! thanks!

hawkw · 2022-05-23T21:09:49Z

tokio-console/src/config.rs

+ #[clap(name = "install", long = "install")]
+ install: bool,
+ #[clap(arg_enum)]
+ shell: Shell,


i wonder if it's worthwhile to also try to read this from the value of the $SHELL environment variable, if it's not provided by the user? we could add that in a follow-up.

<a name="0.1.6"></a> ## 0.1.6 (2022-05-23) #### Features * add `Builder::poll_duration_histogram_max` (tokio-rs#351) ([a966feb](a966feb)) #### Bug Fixes * fix memory leak from resizing histograms (tokio-rs#351) ([32dd337](32dd337), closes [tokio-rs#350](350))

<a name="0.1.6"></a> ## 0.1.6 (2022-05-24) #### Bug Fixes * default `--no_colors` to `false` (#344) ([e58352f](e58352f)) #### Features * add subcommand to gen shell completions (#336) ([df4d468](df4d468)) * display outliers in histogram view (#351) ([dec891f](dec891f))

feat(console): add subcommand for gen completions

d5be537

nrskt requested a review from a team as a code owner April 26, 2022 18:06

hawkw reviewed Apr 26, 2022

View reviewed changes

hawkw reviewed Apr 30, 2022

View reviewed changes

nrskt added 2 commits May 2, 2022 15:37

chore(console): fix to parsing subcmd

b51ce10

chore(console): fix comment for gen-completion

60fbce5

nrskt added 3 commits May 2, 2022 16:58

chore(console): fix wrong comment

fa91ca9

chore(console): add install option to gen-completion

ca64108

chore(console): fix clippy

4bf1f96

nrskt commented May 2, 2022

View reviewed changes

nrskt requested a review from hawkw May 3, 2022 20:12

hawkw reviewed May 5, 2022

View reviewed changes

nrskt added 4 commits May 7, 2022 14:47

chore(console): disable auto install completion

9bb401d

chore(console): call generate function using prefix

0c02ebf

chore(console): remove sample documentation comment

0896e30

chore(console): move to config module

99f659f

nrskt requested a review from hawkw May 7, 2022 06:32

hawkw approved these changes May 23, 2022

View reviewed changes

hawkw enabled auto-merge (squash) May 23, 2022 21:11

hawkw added 3 commits May 23, 2022 14:11

Merge branch 'main' into generate_completions

75d0afb

Merge branch 'main' into generate_completions

2057281

hawkw force-pushed the main branch from b1128bd to 95a17b6 Compare May 23, 2022 21:34

Merge branch 'main' into generate_completions

ed1c43a

hawkw merged commit df4d468 into tokio-rs:main May 23, 2022

hawkw mentioned this pull request May 24, 2022

chore(console): prepare to release v0.1.6 #357

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(console): add subcommand for gen completions #336

feat(console): add subcommand for gen completions #336

nrskt commented Apr 26, 2022

hawkw Apr 26, 2022

nrskt Apr 28, 2022 •

edited

Loading

hawkw Apr 30, 2022

hawkw Apr 30, 2022

nrskt May 2, 2022

hawkw Apr 30, 2022

nrskt May 2, 2022

hawkw Apr 30, 2022

nrskt commented May 2, 2022

nrskt May 2, 2022 •

edited

Loading

hawkw May 5, 2022

nrskt May 2, 2022 •

edited

Loading

hawkw May 5, 2022

hawkw left a comment

hawkw May 5, 2022

nrskt May 7, 2022

hawkw May 5, 2022

hawkw May 5, 2022

nrskt May 7, 2022

hawkw May 5, 2022

hawkw May 5, 2022

nrskt commented May 7, 2022

hawkw left a comment

hawkw May 23, 2022

	generate(shell, &mut app, "tokio-console", &mut buf);
	clap_complete::generate(shell, &mut app, "tokio-console", &mut buf);

	_ => color_eyre::eyre::bail!("Not support to install completion script on {}", shell),
	_ => color_eyre::eyre::bail!("Automatically installing completion scripts is not currently supported on {}", shell),

feat(console): add subcommand for gen completions #336

feat(console): add subcommand for gen completions #336

Conversation

nrskt commented Apr 26, 2022

Choose a reason for hiding this comment

nrskt Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrskt commented May 2, 2022

nrskt May 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrskt May 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrskt commented May 7, 2022

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrskt Apr 28, 2022 •

edited

Loading

nrskt May 2, 2022 •

edited

Loading

nrskt May 2, 2022 •

edited

Loading